New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix limit check on variables with double accuracy #1047
Conversation
Fix double check accuracy.
Update command.c
Can you give a little more information about this? This code has been stable since 2013, so I think we need some sort of evidence to change it. |
@dngarrett : You were in here last (in 2017). Do you have any thoughts? |
I have a really small mill, once I put a rotary table on it I'm really using all the space on it. After applying this patch my tool changing g-code is workable, and the machine retracts to 80mm; it will complain if it's 80.1mm (as it is supposed to be), but 80 itself is ok. |
As a reference. we're dealing with 10^0-10^2 or 3 so the accuracy should be around 15 for double. Within the last 10 years I only had to deal with such high precision once as far as I remember. I guess most programmers would have to read up about that topic to get into it again. |
This may the reason I have always set my max limit 0.001mm more than what I wanted and my min limit 0.001mm less than what I wanted so I could avoid that problem. |
The eps setting from the 2017 commit: The pull request's suggested tests seem a little awkward. (i have not been able to reproduce the problem in a sim |
Well you know the bug feel free to fix it :-) I'm out here. Read the double documentation again and try to understand it next time. |
please also fix inRange in command.c same story here. I guess the entire emc/linuxcnc project has to be checked for double accuracy issues.... /* inRange() returns non-zero if the position lies within the joint
sometimes it works sometimes it doesn't, depending on the value determined by the tool setter code. |
.... next one: A screenshot of the output: |
I don't understand this. Whilst I am clear on the fact that double does not have an exact representation for every real number, it should still be the case that < and > should work for comparing two doubles set to the same real value. In the screenshot the axis position is outside the axis limits, albeit by a tiny amount. I would tend to the opinion that the problem lies with the code which is calculating these out-of-limit values, not with the bounds checking itself. |
certainly the double inaccuracy is also within the calculation too, but the low values shouldn't be used for the check at all either. G53 G01 Z80 Fails and the result is that picture (and other errors, this discussion already shows 3 occurrences in the code where it's an issue). I can go to Z79.9999999 via G-Code, but not to Z80.000 However jogging to Z80 via PAGEUP was no problem. I've been told that some people tend to use a limit of Z80.001 because they thought that the limit is exclusive the value given in the configuration file, no it is inclusive but the double accuracy issue avoids that. It has been like that for many years it seems. I'm using the updated version since I reported it and did not experience any issue anymore. |
OK, I agree that if the G-code says "80" and the INI says "80" then there should not be a problem. But I am puzzled as to why the two places would interpret the string "80" as two different values. As far as I know the G-code interpreter and HAL both use double-precision throughout. |
I understand what you mean, double calculations have to be checked in the entire linuxcnc project. I see many if ... > or < checks, but nothing that indicates that the double precision ever has been taken care about. The procedure to get to that was:
So either there's a memory corruption or cmd_pos will be recalculated from that point on, but that should not affect movements in G53 (machine coordinate system) |
Thanks @McCodie for your efforts to improve LinuxCNC. @andypugh asked me to drop in and give my opinion. In floating point, there are many numbers A and B for which the usual identity (There are also sometimes surprising units conversions in the software -- I forget if setting I'm not categorically against such changes, but I'm also not against better documenting the advised practice of setting the soft limits just outside the intended usable volume of the machine, because @McCodie is almost certainly right that this is not the only instance where attempting to go "exactly to" a limit will not work out consistently depending on how offsets combine & cancel. (note: I took a search through the docs and didn't find where it's clearly spelled out, though from the comments in this thread -- thanks @phillc54 ! -- it's clear that this is at least "folk knowledge" among some users) Another reason for setting soft limits slightly outside of the "intended limits" is feedback position: Both with servo feedback & with steppers there are (A) another set of scalings & roundings and (B) the possibility that either the physical servo OR the stepper velocity ramp will lead to a move INTENDED to terminate exactly at the "limit" be reported as going some counts beyond it. (also, this folk knowledge is NOT followed in sim/axis.ini but it is in stepconf, but only if the home position is right up against the soft limit: # linuxcnc doesn't like having home right on an end of travel,
# so extend the travel limit by up to .01in or .1mm
minlim = get("minlim")
maxlim = get("maxlim")
home = get("homepos")
if self.d.units: extend = .001
else: extend = .01 (also I think the comment's wrong, it must be .001in or .01mm...) |
So, it seems that I was wrong in my assumptions, and jepler thinks that you are on to something. You definitely should be able to move to a point that displays the number that you configured as the limit. I feel that your condition: Could be re-stated as
And the same with a + for the positive limit. I wonder what the worst-case error is, and if it can ever accumulate? |
I have rebased this against 2.8 (as a bug fix) and pushed it. |
This fixes a bug which avoided to be able to drive to the Z limit after a tool change via G-Code in Axis.
I created an issue:
#1046